xdsresolver: use the refcounted utility for cluster refcounting - #9318
xdsresolver: use the refcounted utility for cluster refcounting#9318ulascansenturk wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9318 +/- ##
==========================================
+ Coverage 87.54% 87.56% +0.02%
==========================================
Files 429 429
Lines 30622 30618 -4
==========================================
+ Hits 26807 26810 +3
+ Misses 3814 3807 -7
Partials 1 1
🚀 New features to boost your workflow:
|
105fcbc to
c8266c9
Compare
easwars
added
Type: Internal Cleanup
c8266c9 to
a8b9135
Compare
Replace the hand-rolled atomic.Int32 in clusterInfo with grpcsync.RefCounted. The "decrement, and if the count hit zero run the cleanup" logic was duplicated at three sites; it now lives in a single onZero callback registered when the entry is created. Entries remove themselves from activeClusters/activePlugins when their last reference is released, so pruneActiveClustersAndPlugins is no longer needed. Removal is scheduled on the serializer, since the last reference is usually released by an RPC completing on an arbitrary goroutine, and is guarded by an identity check so a pending removal cannot evict a newer entry that has since taken the same key. References for a config selector are now taken as each cluster is recorded rather than in a batch after all routes are built, so a config selector that fails partway through releases exactly what it acquired. Each routeCluster now holds the refcounted entry it refers to. Config selection no longer looks the entry up by name in cs.clusters/cs.plugins on the RPC path, which also removes the unreachable panic for a matched cluster missing from both maps. RELEASE NOTES: none
a8b9135 to
9c11b34
Compare
|
Hey @ulascansenturk, could you please reply to the comments once you've addressed them? Also, when updating the PR, please push your new changes as separate commits rather than force-pushing to squash them. Keeping the commit history intact makes it much easier for us to review what has changed. Thanks! |
| @@ -1385,6 +1385,95 @@ func (s) TestResolverKeepWatchOpen_ActiveRPCs(t *testing.T) { | |||
| // TestResolver_XDSConfigInRPCContext verifies that the xDS resolver's config | |||
There was a problem hiding this comment.
This the comment has been seperated from the test , please fix.
There was a problem hiding this comment.
Fixed. The doc comment for TestResolver_XDSConfigInRPCContext had been left above my test, so both were orphaned. Moved it back onto its own function.
| // single reference per distinct cluster, however many routes name it, and | ||
| // releases exactly that one reference when it is stopped. | ||
| // | ||
| // If a reference were taken per route instead, the counts would not balance: |
There was a problem hiding this comment.
I think we can remove this paragraph. We is being done and tested should be enough.
| // TestActivePluginRefCounting verifies that repeated acquisitions of a cluster | ||
| // specifier plugin share a single entry, and that the entry is removed from | ||
| // activePlugins only once the last reference is released. | ||
| func (s) TestActivePluginRefCounting(t *testing.T) { |
There was a problem hiding this comment.
THis can easily be converted to an E2E test where we send RPCs on a plugin and make sure it is not removed from the service config when the RPCs are in flight. ANd is removed once the RPCs are done and it is no longer part of the new XDSConfig.
There was a problem hiding this comment.
Dropped this test entirely. TestXDSResolverDelayedOnCommittedCSP already covers that scenario end to end: it holds an RPC on cspA, switches the route to cspB, checks both appear in the service config, then commits the RPC and checks cspA is gone. Adding another e2e test for it would just duplicate that one.
| // count has already dropped to zero is replaced by a fresh entry rather than | ||
| // resurrected, and that the pending removal of the dead entry does not delete | ||
| // its replacement. | ||
| func (s) TestActivePluginNotRevivedAfterRelease(t *testing.T) { |
There was a problem hiding this comment.
Can we also try to turn this into an e2e test? We might not be able to test the exact same thing but we should be able to test the concurrency , something like if last RPC ref is committed in a go routine and we send the xdsConfig update at the same time, whichever happens first should be handled correctly. WDYT ?
There was a problem hiding this comment.
Done, added TestResolverClusterSpecifierPluginRefCountRace in cluster_specifier_plugin_test.go and removed the unit test.
It holds an RPC on cspA, moves the route to cspB so cspA is referenced only by that RPC, then commits the RPC in a goroutine while pushing an update that names cspA again. Both orderings have to converge on cspA being in the service config and usable for new RPCs: if the commit wins, the entry is torn down and TryIncrement fails so a fresh one replaces it, and if the update wins the existing entry is reused. Passes with -race at -count=10.
I also added a waitForServiceConfig helper, since the resolver publishes intermediate configs here and verifyUpdateFromResolver only looks at the next update.
There was a problem hiding this comment.
Follow up: the first version of this test was flaky and CI caught it. It pushed the cspB update and the cspA update back to back without waiting in between, so on a loaded machine the client could be scheduled only after both snapshots had landed. The route config it then saw was identical to the original cspA one, so the resolver had nothing new to publish and the test timed out. Reproducible locally with -cpu 1.
It now waits for the intermediate config naming both plugins before starting the race, which confirms the client processed cspB first. Passing at -count=20 with -cpu 1, and with -race at -cpu 1, 2 and 4.
|
Heads up on the red The identical failure is on #9343 and a couple of other open PRs, and master's own codecov runs were green through the 19th, which lines up with when I could not find an existing issue for it. I am happy to open one and fix it if nobody is already on it. It looks like the hardcoded sizes in the otel and csm tests want computing at runtime rather than asserting a literal, otherwise it breaks again on the next flate change. |
Hey , thank you for investigating this, we are already in discussion about the best way to fix this. |
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
|
Hey @ulascansenturk , can you rebase it on master to get the codecov job passing ? |
|
Done, merged master in rather than rebasing so the existing commits stay intact, as you asked earlier. That picks up #9346, and the otel and csm packages now pass locally under the same Also reran the full |
| }) | ||
| } | ||
|
|
||
| type clusterInfo struct { |
There was a problem hiding this comment.
Now that this only has one field, can we remove the struct and just use xdsChildConfig instead ?
| // info is the resolver-wide entry for this cluster, shared by every route | ||
| // that references it. An RPC routed here holds a reference on it until the | ||
| // RPC is committed. | ||
| info *grpcsync.RefCounted[*clusterInfo] |
There was a problem hiding this comment.
Why do we need to store it here ? We can retrieve it from the configSelector struct itself , similar to what was being done earlier. Storing it in different places increases risk and overhead of making sure they are all synced.
3 participants
Replaces the hand-rolled
atomic.Int32inclusterInfowithgrpcsync.RefCounted, as suggested in #9304.The "decrement, and if the count hit zero run the cleanup" logic was duplicated at three sites (
SelectConfig'sOnCommitted,configSelector.stop, andpruneActiveClustersAndPlugins). It now lives in a singleonZerocallback registered when the entry is created, which letsSelectConfig's cluster and plugin branches collapse into one path.Entries now remove themselves from
activeClusters/activePluginsonce their last reference is released, sopruneActiveClustersAndPluginsis gone.Two details worth a reviewer's attention:
The map removal is scheduled on the serializer, because the last reference is usually released by an RPC completing on an arbitrary goroutine while the active maps may only be touched from a serializer callback. It is guarded by an identity check so that a pending removal cannot evict a newer entry that has since taken the same key, and acquisition uses
TryIncrementso a dead entry is replaced rather than revived.For clusters, the removal is queued before
unsubscribe()is called, andunsubscribe()itself stays synchronous. Queueing first matters because unsubscribing makes the dependency manager push an update onto the same serializer; if the removal were queued after, that update would regenerate a service config still containing the dropped cluster (TestResolverKeepWatchOpen_ActiveRPCscatches this). Keepingunsubscribe()out of the scheduled callback matters because that callback returns early when a newer entry has taken the key, which would otherwise leak the old subscription.References for a config selector are now taken as each cluster is recorded, rather than in a batch after all routes are built. This keeps acquisition and release symmetric, so a config selector that fails partway through releases exactly what it acquired.
TestPruneActiveClustersis replaced by two tests covering entry reuse/release and the no-revival guard.Fixes #9304
RELEASE NOTES: none